Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tolino Epos 1 warmth lights support #513

Merged
merged 2 commits into from
Sep 30, 2024
Merged

Conversation

hugleo
Copy link
Contributor

@hugleo hugleo commented Sep 27, 2024

Manufacturer: rakuten kobo inc
Brand: rakutenkobo
Model: tolino
Device: ntx_6sl
Product: ntx_6sl
Hardware: e70q20
Platform: imx6
Friendly name:
test.log
Tolino Epos 1
Android 4.4
Tolino Ntx, Freescale/NTX

This change is Reviewable

@Frenzie Frenzie requested a review from pazos September 27, 2024 10:39
Copy link
Member

@pazos pazos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The device is already recognized as a TOLINO_EPOS, which is a shared definition that applies to EPOS1 and EPOS2. The only change you'll need to apply is the light driver, during factory.

Both devices should benefit from Root to Ntx transition.

Copy link
Contributor Author

@hugleo hugleo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already is:

            return when (DeviceInfo.LIGHTS) {
                DeviceInfo.LightsDevice.NOOK_GL4,
                DeviceInfo.LightsDevice.TOLINO_EPOS -> {
                    logController("TolinoRoot")
                    TolinoRootController()
                }

That's because I follow the model from Tolino Shine 3 because for Tolino Epos 1 TolinoRoot doesn't work and the one that works is TolinoNTX

// Tolino Shine 3 also has warmth lights, but with ntx_io file
        TOLINO_SHINE3 = BRAND.contentEquals(STR_KOBO)
            && MODEL.contentEquals(STR_TOLINO)
            && DEVICE.contentEquals(STR_NTX)
            && HARDWARE.contentEquals("e60k00")
                DeviceInfo.LightsDevice.CREMA_0710C,
                DeviceInfo.LightsDevice.CREMA_CARTA_G,
                DeviceInfo.LightsDevice.MEEBOOK_P6,
                DeviceInfo.LightsDevice.RIDI_PAPER_3,
                DeviceInfo.LightsDevice.TOLINO_EPOS1,
                DeviceInfo.LightsDevice.TOLINO_SHINE3,
                DeviceInfo.LightsDevice.TOLINO_VISION4,
                DeviceInfo.LightsDevice.TOLINO_VISION5 -> {
                    logController("TolinoNTX")
                    TolinoNtxController()
                }

@pazos
Copy link
Member

pazos commented Sep 27, 2024

Yes, please move DeviceInfo.LightsDevice.TOLINO_EPOS from TolinoRootController() to TolinoNtxController()

Don't add new identifiers here, Tolino 1 and 2 share epd and lights drivers.

///--///

Or, if you want to document Epos1, create a new definition and use that for both lights and epd. Also rename EPOS to EPOS2

@hugleo
Copy link
Contributor Author

hugleo commented Sep 27, 2024

Yes, please move DeviceInfo.LightsDevice.TOLINO_EPOS from TolinoRootController() to TolinoNtxController()

Don't add new identifiers here, Tolino 1 and 2 share epd and lights drivers.

///--///

So, we would do right now then because the following implement exactly like a do :)

        && !HARDWARE.contentEquals("e60k00")
        && !HARDWARE.contentEquals("e60q50")
        && !HARDWARE.contentEquals("e70k00")

Then remove TOLINO_SHINE3, TOLINO_VISION4, TOLINO_VISION5 as well, why not?

Or, if you want to document Epos1, create a new definition and use that for both lights and epd. Also rename EPOS to EPOS2

@pazos
Copy link
Member

pazos commented Sep 28, 2024

Then remove TOLINO_SHINE3, TOLINO_VISION4, TOLINO_VISION5 as well, why not?

As I said previously I have no problem with different identifiers for EPOS1 and EPOS2, as long as you use those identifiers to asign proper eink drivers to each one.

The current PR removes the device from being identified as a EPOS, so no longer receives TolinoNTX as its controller.

@hugleo
Copy link
Contributor Author

hugleo commented Sep 28, 2024

Then remove TOLINO_SHINE3, TOLINO_VISION4, TOLINO_VISION5 as well, why not?

As I said previously I have no problem with different identifiers for EPOS1 and EPOS2, as long as you use those identifiers to asign proper eink drivers to each one.

The current PR removes the device from being identified as a EPOS, so no longer receives TolinoNTX as its controller.

For EDP driver is not being removed because is catched again here MODEL.contentEquals(STR_TOLINO), worth for all TOLINOS:
EDIT: Is not catched, I misread some parentheses

        TOLINO = BRAND.contentEquals(STR_TOLINO) && MODEL.contentEquals("imx50_rdp")
            || MODEL.contentEquals(STR_TOLINO) && (DEVICE.contentEquals("tolino_vision2")
            || DEVICE.contentEquals(STR_NTX))

In EPDFactory.kt there is no TOLINO_EPOS.

Seems like TOLINO_EPOS is behaving like a generic light driver because is used only in LightsFactory.kt for TolinoRootController.
ref: #239 (comment)

I'll rename TOLINO_EPOS to TOLINO_EPOS2 and TOLINO_EPOS1 add to both EPD and lights driver.

@pazos
Copy link
Member

pazos commented Sep 29, 2024

For EDP driver is not being removed because is catched again here MODEL.contentEquals(STR_TOLINO), worth for all TOLINOS:
EDIT: Is not catched, I misread some parentheses

Ah, my bad. You're right.

Let me see if I can search if the EPOS2 was reported once and split those two devices properly without TOLINO_EPOS being the tolino that's not all these other modern tolinos.

@pazos
Copy link
Member

pazos commented Sep 29, 2024

Nope. It turns out it was a major @zwim contribution. AFAICT he no longer has the device.

As far as this PR: please do as you like:

  1. Stick with TOLINO_EPOS for both 1 and 2 and move it to "lights ntx"
  2. Create the new TOLINO_EPOS1 as you did, rename the old one to TOLINO_EPOS2 and move it to "lights ntx" too.
  3. Create the new TOLINO_EPOS1 as you did, remove all the references to TOLINO_EPOS and wait for a future report that lets us to identify that device properly.

@hugleo
Copy link
Contributor Author

hugleo commented Sep 29, 2024

Nope. It turns out it was a major @zwim contribution. AFAICT he no longer has the device.

As far as this PR: please do as you like:

1. Stick with `TOLINO_EPOS` for both 1 and 2 and move it to "lights ntx"

2. Create the new `TOLINO_EPOS1` as you did, rename the old one to `TOLINO_EPOS2` and move it to "lights ntx" too.

3. Create the new `TOLINO_EPOS1` as you did, remove all the references to `TOLINO_EPOS` and wait for a future report that lets us to identify that device properly.

3

@Frenzie
Copy link
Member

Frenzie commented Sep 29, 2024

Ready to merge?

@hugleo
Copy link
Contributor Author

hugleo commented Sep 30, 2024

Listo

@pazos
Copy link
Member

pazos commented Sep 30, 2024

Yep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants